MPT-22435 derive product-document e2e fixtures from a created product#351
Conversation
…PT-22435) The catalog product-document e2e tests created documents against a hardcoded seeded product (catalog.product.id / PRD-7255-3950). That shared product accumulated documents across runs until it reached the server-side per-product maximum, after which document creation failed at fixture setup with "400 ... maximum number of documents has been reached", turning the suite red. Point the document-service fixtures at a freshly created product per test: - Hoist created_product / async_created_product into the product conftest so the documents subpackage can use them. - vendor_document_service / async_vendor_document_service now build off created_product.id / async_created_product.id. - Rework the get/download tests to create their own document (dropping the seeded document_id) and assert the uploaded file name. - Consolidate the duplicated async document-service fixture. Verified: scoped e2e run green (32 passed), make check clean, unit suite 2310 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Found Jira issue key in the title: MPT-22435 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🧰 Additional context used📓 Path-based instructions (2)**/*⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (7)📚 Learning: 2025-12-12T15:02:20.732ZApplied to files:
📚 Learning: 2026-04-02T09:35:03.825ZApplied to files:
📚 Learning: 2026-01-08T08:34:05.465ZApplied to files:
📚 Learning: 2026-01-08T23:38:19.565ZApplied to files:
📚 Learning: 2026-02-02T13:05:41.144ZApplied to files:
📚 Learning: 2026-04-16T13:00:41.320ZApplied to files:
📚 Learning: 2026-01-08T08:34:19.306ZApplied to files:
🔇 Additional comments (5)
📝 WalkthroughWalkthrough
ChangesProduct fixture centralization and helper enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
albertsola
left a comment
albertsola
left a comment
There was a problem hiding this comment.
We have a set of helpers to simplify all that code
| @@ -43,37 +38,47 @@ def test_create_from_link_async(created_document_from_link_async, pdf_url, docum | |||
| assert created_document_from_link_async.description == document_data["description"] | |||
|
|
|||
There was a problem hiding this comment.
we have helpers that does this for you:
check async_create_fixture_resource_and_delete and create_fixture_resource_and_delete
There was a problem hiding this comment.
Done in 3fee2a1 — both fixtures (sync + async) now use create_fixture_resource_and_delete / async_create_fixture_resource_and_delete. I extended those helpers with an optional upload_file param so the file-backed document fixture can use them too (backward compatible: it is only forwarded to create() when provided, so existing non-file callers are unchanged). 🤖 Generated by AI
| async def test_iterate_documents_async( | ||
| async_vendor_document_service, created_document_from_file_async | ||
| ): | ||
| documents = [doc async for doc in async_vendor_document_service.iterate()] | ||
|
|
||
| result = any(doc.id == created_document_from_file_async.id for doc in documents) | ||
|
|
||
| assert result is True |
There was a problem hiding this comment.
check helper: assert_service_filter_with_iterate and assert_async_service_filter_with_iterate
There was a problem hiding this comment.
Done in 3fee2a1 — test_filter_documents / test_filter_documents_async now use assert_service_filter_with_iterate / assert_async_service_filter_with_iterate. 🤖 Generated by AI
| async def test_update_document_async( | ||
| async_vendor_document_service, created_document_from_file_async | ||
| ): | ||
| update_data = {"name": "Updated e2e test document - please delete"} | ||
|
|
||
| result = await async_document_service.update(created_document_from_file_async.id, update_data) | ||
| result = await async_vendor_document_service.update( | ||
| created_document_from_file_async.id, update_data | ||
| ) | ||
|
|
||
| assert result.name == update_data["name"] |
There was a problem hiding this comment.
check helper: assert_update_resource and assert_async_update_resource
There was a problem hiding this comment.
Done in 3fee2a1 — test_update_document / test_update_document_async now use assert_update_resource / assert_async_update_resource. 🤖 Generated by AI
Address review feedback on #351: replace hand-rolled fixture/teardown and assertion boilerplate with the shared tests/e2e/helper.py helpers. - created_document_from_file/_url (sync + async) now use create_fixture_resource_and_delete / async variant. - Extend those helpers with an optional upload_file so file-backed document creation can use them (backward compatible: forwarded only when provided). - test_update_document(_async) -> assert_update_resource / async variant. - test_filter_documents(_async) -> assert_service_filter_with_iterate / async. Verified: scoped e2e green (20 passed), make check clean, unit suite 2310 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
2 participants
What
The catalog product-document e2e tests now create their own product per test instead of reusing the hardcoded seeded product (
catalog.product.id/PRD-7255-3950).created_product/async_created_productintotests/e2e/catalog/product/conftest.pyso thedocuments/subpackage can use them.vendor_document_service/async_vendor_document_servicenow build offcreated_product.id/async_created_product.id.document_id); the download assertion now checks the uploaded file name (empty.pdf).Why
The seeded product is shared across runs. Documents accumulated on it over time until it reached the server-side per-product maximum, after which document creation failed at fixture setup:
Creating a fresh product per test starts each test from zero documents, eliminating the limit failure and removing the dependency on shared seeded state. Same pattern as #339 (order asset fixtures derived from a created order).
Fixes MPT-22435.
Reviewer notes
empty.pdf) rather than the old seededpdf - empty.pdf.components/fixVersionsare required on the linked Jira bug; set toExtension Python SDK/v6.Verification
tests/e2e/catalog/product/documents+ the two product test modules).make check(ruff format/check, flake8, mypy, uv lock) clean.🤖 Generated with Claude Code
Closes MPT-22435
Release Notes
created_productandasync_created_productfixtures totests/e2e/catalog/product/conftest.pyso document subpackage tests can reuse dynamically created productsvendor_document_serviceandasync_vendor_document_servicefixtures to derive service context from the dynamically created product IDsdocument_id) and updated download assertions to expectempty.pdftests/e2e/helper.py, removing hand-rolled lifecycle/teardown and standardizing assertionsupload_fileparameter to support file-backed document creation